Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Check for SDF files containing sibling elements with the same name (Forward port of #3016) #3017

Merged
merged 5 commits into from
Jun 9, 2021
Merged

Check for SDF files containing sibling elements with the same name (Forward port of #3016) #3017

merged 5 commits into from
Jun 9, 2021

Conversation

j-rivero
Copy link
Contributor

@j-rivero j-rivero commented Jun 4, 2021

Forward port of #3016.

Gazebo11 supports SDF 1.7 which is different of #3016 since it only support SDF 1.6. I had to comment and reverse the expectations in one of the tests since it is affected by gazebosim/sdformat#586 from libsdformat9.

@j-rivero
Copy link
Contributor Author

j-rivero commented Jun 7, 2021

CI has some problems:

@j-rivero j-rivero requested a review from jennuine June 7, 2021 18:22
@j-rivero
Copy link
Contributor Author

j-rivero commented Jun 8, 2021

abichecker is complaining about rendering - LensFlare.hh, unrelated to this PR but needs checking

I've looked into it. #3007 seems the one triggering the problem but that PR is adding the symbol and not removing it, so it should be a bug in our abi checker code or in the underlying abi-compliance-checker tool. Not affecting this PR.

@j-rivero j-rivero changed the title Check for SDF files containing sibling elements with the same name (Forward port of #3019) Check for SDF files containing sibling elements with the same name (Forward port of #3016) Jun 8, 2021
jennuine
jennuine reviewed Jun 8, 2021
View reviewed changes
Copy link
Collaborator

@jennuine jennuine left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me but isn't Windows complaining from unsetenv not being defined?

C:\Jenkins\workspace\gazebo-ci-pr_any-windows7-amd64\ws\gazebo\test\integration\sdf_errors.cc(117): error C3861: 'unsetenv': identifier not found

Also, can the new environment variable GAZEBO11_BACKWARDS_COMPAT_WARNINGS_ERRORS be documented somewhere?

@j-rivero
Copy link
Contributor Author

j-rivero commented Jun 8, 2021

This looks good to me but isn't Windows complaining from unsetenv not being defined?

Ouch, right. Should be fixed in 4db4697

Also, can the new environment variable GAZEBO11_BACKWARDS_COMPAT_WARNINGS_ERRORS be documented somewhere?

Forget to port the change in Changelog, ready in 0987978

Gazebo11 supports SDF 1.7 which is different of #3016 since it only support SDF 1.6. I had to comment and reverse the expectations in one of the tests since it is affected by osrf/sdformat#586 from libsdformat9.

I've implemented the logic to handle this case in 0219c47 since I'm not sure if the method description in sdformat fit well about checking SDF versions requirements. We can follow the discussion in the sdformat issue but this implementation should not hurt in anyway I think.

@j-rivero j-rivero requested a review from jennuine June 8, 2021 23:02
jennuine
jennuine approved these changes Jun 9, 2021
View reviewed changes
Copy link
Collaborator

@jennuine jennuine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. CI failures don't appear to be related to this PR but could use a second pair of eyes

@j-rivero
Copy link
Contributor Author

j-rivero commented Jun 9, 2021

LGTM. CI failures don't appear to be related to this PR but could use a second pair of eyes

Looking into them:

The only ones I've detected that are somehow 'new' (not in the daily build or not in other builds) are:

  • INTEGRATION_transport.test_ran unrelated to SDF management
  • UNIT_SonarSensor_TEST.test_ran: segfaulting, there is a warning that applying negative mass can end up in a segfault :(

@j-rivero
Copy link
Contributor Author

j-rivero commented Jun 9, 2021

Brew CI seems generally broken right now. Going to merge to have the release out since previous builds were fine as far as I can tell.

@j-rivero j-rivero merged commit 6bc0f63 into gazebo11 Jun 9, 2021
@j-rivero j-rivero deleted the sdf_check_gazebo11 branch June 9, 2021 12:11
@jennuine jennuine mentioned this pull request Jun 9, 2021
Merged
@j-rivero j-rivero mentioned this pull request Jun 11, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jennuine jennuine jennuine approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@j-rivero @jennuine